chore!: improve ChunkProofVerification #2446
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
maqi
force-pushed
the
improve_chunk_proof_verification
branch
2 times, most recently
from
2f72a3d to
1890cf5
Compare
maqi
changed the title
sn_node/src/node.rs
Outdated
| let mut all_chunk_addrs: Vec<_> = all_local_records | ||
| .iter() | ||
| .filter_map(|(addr, record_type)| { | ||
| if *record_type == RecordType::Chunk { |
There was a problem hiding this comment.
Why only chunks? I think all records should be checked.
There was a problem hiding this comment.
register/spend has nature of could have different versions under the same name,
which, it's not guaranteed to be always consistence across the network.
they are also small in generally cases, which cheaper to be cached, and include them could pollute the scoring ?
There was a problem hiding this comment.
They can be self verifiable, so even if different, the sig is what matters
There was a problem hiding this comment.
If we only verify storage of chunks what's my incentive as a node to keep other records?
We should check all records regardless of what type.
There was a problem hiding this comment.
They can be self verifiable, so even if different, the sig is what matters
Unfortunately, they are not.
To verify the signature, the owner's pub key must be known.
But nodes doesn't have that info, unless being contacted by the client.
suggested to have client check all types of records
maqi
force-pushed
the
improve_chunk_proof_verification
branch
from
1890cf5 to
3dfd466
Compare
maqi
force-pushed
the
improve_chunk_proof_verification
branch
from
3dfd466 to
1eb10ae
Compare
| // Sort by distance and only take first X closest entries | ||
| all_chunk_addrs.sort_by_key(|addr| key.distance(addr)); | ||
|
|
||
| // TODO: this shall be deduced from resource usage dynamically |
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note
|
|
||
| let index: usize = OsRng.gen_range(0..num_of_targets); | ||
| let target = verify_candidates[index].clone(); | ||
| // TODO: workload shall be dynamically deduced from resource usage |
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note
| Ok((peer_id, score)) => { | ||
| if score < MIN_ACCEPTABLE_HEALTHY_SCORE { | ||
| info!("Peer {peer_id:?} failed storage challenge with low score {score}/{MIN_ACCEPTABLE_HEALTHY_SCORE}."); | ||
| // TODO: shall the challenge failure immediately triggers the node to be removed? |
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note
| } | ||
| // TODO: For those answers not among the expected_proofs, |
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note
sn_node/src/node.rs
Outdated
| let mut all_chunk_addrs: Vec<_> = all_local_records | ||
| .iter() | ||
| .filter_map(|(addr, record_type)| { | ||
| if *record_type == RecordType::Chunk { |
There was a problem hiding this comment.
They can be self verifiable, so even if different, the sig is what matters
sn_node/src/node.rs
Outdated
| let mut all_chunk_addrs: Vec<_> = all_local_records | ||
| .iter() | ||
| .filter_map(|(addr, record_type)| { | ||
| if *record_type == RecordType::Chunk { |
There was a problem hiding this comment.
If we only verify storage of chunks what's my incentive as a node to keep other records?
We should check all records regardless of what type.
| } | ||
|
|
||
| pub fn historical_verify(&self, _other: &Self) -> bool { | ||
| // TODO: Shall be refactored once new quote filtering scheme deployed |
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note
| // old_quote.quoting_metrics.close_records_stored | ||
| // ); | ||
|
|
||
| // // TODO: Double check if this applies, as this will prevent a node restart with same ID |
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note
maqi
force-pushed
the
improve_chunk_proof_verification
branch
from
9386241 to
9a922db
Compare
maqi
force-pushed
the
improve_chunk_proof_verification
branch
from
9a922db to
9c9f64f
Compare
maqi
force-pushed
the
improve_chunk_proof_verification
branch
from
9c9f64f to
57eb2c3
Compare
Description
🚨 Breaking Changes
improve ChunkProofVerification